feat!: extend options accumulate instead of replacing - #49
Merged
Conversation
`extend`'s return type spelled the merge inline as `Omit<A, keyof A2> & A2`. TypeScript 5.9.3 copied the type parameter `A2` through unsubstituted whenever `A` was `Record<never, never>` — a root declaring no `computed`, the default — leaving a dangling name in the consumer's own declarations, which then failed with `TS2304: Cannot find name 'A2'`. 7.0.2 substitutes the same position correctly, so only downstream builds saw it. Hoist the merge into an exported `MergedComputed<A, A2>`, top-level in `index.ts` and named in the `Entity` namespace for the same emit-nameability reason as `EntityStatic` — unexported it would only trade `TS2304` for `TS4023`. Nothing caught this because the consumer gate emitted declarations and stopped there: `TS4020` is an emit-time diagnostic, a dangling reference in the *output* is not. `typecheck` now feeds `node_modules/.emit-check` back through the 5.9.3 compiler, with no `--skipLibCheck` — measured, that flag makes the step exit 0 on the broken output.
`BillingDocumentBase` declared none, so the two-compiler declaration pass only ever evaluated `MergedComputed` at `A = Record<never, never>` — the branch that costs nothing. `period`, the accounting period derived from `issuedAt`, puts a real map on the root, so the root's schemas serialise into every variant's `.d.ts` and the `TS7056` margin is measured where it is actually spent. Both compilers emit clean and the emitted output type-checks on 5.9.3. Real modelling rather than a stub: billing reports and revenue recognition both work per period, and deriving it is what stops a stored copy disagreeing with the date it came from.
A variant redefining a root's computed key gets its own type on `Entity.Output`, but the *instance* keeps the root's brand intersected in, because a root's instance type is carried unmapped and subtracting from it is what `TS2425` forbids. That was described in prose beside the test and asserted nowhere, so the day it changed nothing would have said so.
`docs/reference/types.md` is the canonical statement of the emit-nameability
exception and still said seven. Added `MergedComputed` to the count, the import
block, the namespace-alias sentence and the per-name rationale bullets, and
noted that the fixture now type-checks what it emitted — which is the step that
found this one.
Two off-by-one counts in `entity.ts` ("none of the three", "the same reason as
the three above") follow the same insertion, and its namespace JSDoc said "the
shape `extend` returns" where `extend` returns `EntityStatic<…>`. That one is
published, since TypeDoc renders it for `Entity.MergedComputed`.
Replaced the unmeasured `TS4023` claim in `index.ts` with what was actually
measured. Unexporting the alias and re-running the fixture against a root
declaring no `computed` does not produce `TS4023` — the emitter expands the
alias structurally and the identical dangling `A2` comes back, `TS2304` and all.
So naming it and exporting it are both load bearing, and the `types.ts` comment
no longer implies the name alone is what the emitter writes.
Widened the emitted-declaration step from `index.d.ts` to also name
`emit-guards.d.ts` and `index.spec.d.ts` rather than narrowing rule 2's claim:
nothing imports `emit-guards`, so the emitted form of every namespace member it
names was outside the checked import graph. The wider set is clean, so this
costs nothing and makes the comment true as written.
`index.spec.ts` claimed "on every variant, `CreditNote` included" while
exercising only `Invoice`; it now asserts on both.
There was a problem hiding this comment.
Pull request overview
This PR updates Entity.abstract(...).extend(...) so option inheritance accumulates (root-then-variant) instead of “per-key replace” semantics, removing the silent footgun where variants could accidentally relax immutable/generated constraints and drop root computed entries.
Changes:
- Change the type-level model for
generated/immutablekey tracking from tuples to key unions (PropertyKey) to support accumulation without TS2344 failures. - Update
extendrebuild logic sogenerated,immutable, andinvariantsconcatenate andcomputedmerges per key (child wins per key). - Strengthen the consumer/emit gate by type-checking the emitted
.d.tsoutput (TS 5.9.3), and document/teach the new accumulation rules across docs + examples.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/entity/src/types.ts | Switch key tracking generics to PropertyKey unions; introduce/export MergedComputed and update core derived types accordingly. |
| packages/entity/src/types.test-d.ts | Update type assertions to use key unions instead of key tuples. |
| packages/entity/src/index.ts | Export MergedComputed alongside other declaration-emit names and document why it must remain exported. |
| packages/entity/src/entity.ts | Adapt EntityStatic/factory/update typings to the new key-union model; add namespace alias for MergedComputed. |
| packages/entity/src/base.ts | Implement accumulating option merge in rebuild (concat lists, per-key merge computed). |
| packages/entity/src/base.test-d.ts | Add type-level tests covering “cannot shed inherited immutable” and “computed redefinition isn’t intersected in outputs”. |
| packages/entity/src/base.spec.ts | Update/expand runtime tests to assert accumulation across intermediate roots and per-key computed overrides. |
| examples/billing-domain/src/vocabulary.ts | Add AccountingPeriod branded schema used by the billing domain root’s derived period field. |
| examples/billing-domain/src/root.ts | Add period as a root computed field and document why it’s important for TS7056/consumer gating. |
| examples/billing-domain/src/index.ts | Simplify variants by removing re-statement of inherited generated/immutable keys. |
| examples/billing-domain/src/index.spec.ts | Add tests asserting inherited computed and inherited immutable behavior on variants. |
| examples/billing-domain/src/emit-guards.ts | Expand documentation of the consumer emit-check gate and update empty-key cases from [] to never. |
| examples/billing-domain/package.json | Extend typecheck to compile emitted .d.ts files under the consumer TS version/config. |
| docs/typedoc.json | Exclude MergedComputedSrc from generated API docs, matching other internal “Src” aliases. |
| docs/reference/types.md | Document MergedComputed as an additional declaration-emit name and why it must be exported. |
| docs/reference/declaration.md | Update reference semantics: options accumulate; computed merges per key; relaxing is not expressible. |
| docs/how-to/evolve-an-entity.md | Update guidance to reflect accumulating options and link to the detailed merge behavior section. |
| docs/examples/billing-domain.md | Update example narrative/code to match the new accumulation rules and period computed field. |
| CLAUDE.md | Update repository guidance to reflect accumulating options and the added emitted .d.ts typecheck step. |
| .changeset/extend-options-accumulate.md | Add changeset describing behavior changes, breakages, and migration guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #46. Removes the one merge rule in
extendthat did not do the intuitive thing.The bug
extendmerged options per key, child winning — exceptinvariants, which concatenated. The comment justifying that exception namedimmutableand then did not protect it:So a variant that declared
immutablereplaced the root's list wholesale:issuedAtandissuedTobecame patchable. Nothing reported it. 0.4.0 shipped a comment in the example telling readers they had to re-state every inherited key, and the reference had to teach it. A rule that has to be taught in three places and whose violation is undetectable is better removed than documented.The change
Every option accumulates. Nothing is shed.
invariantsgeneratedimmutablecomputedcomputedis a map rather than a list, so per-key is its analogue of "add, don't shed": a variant adding a derived field keeps the root's, and one redefining a key overrides that entry alone.The declaration syntax does not change. You still write
immutable: ["id"].The type-level part
GandIhad to become unions of keys rather than readonly tuples. The tuple form does not compile:Same failure
types.tsalready recorded forGeneratedOf: TypeScript will not prove the parent's key set is a subset of the child's through zod's inference chain, even though adding fields only adds keys. The union form composes withI | I2[number]and was verified at one level, across a chain, and when the child omits the option entirely.The computed merge is typed
MergedComputed<A, A2>=Omit<A, keyof A2> & A2, notA & A2— the runtime is{ ...parent, ...child }, so a plain intersection would type a redefined key asUpper & Lowerwhile the value isLower.The defect the gate could not see
Writing that merge inline in
extend's return type made TypeScript 5.9.3 emit a dangling type parameter into consumers' declarations whenever the root declares nocomputed— the default shape, and the one this example used:7.0.2 emits the same position correctly, so only the consumer compiler saw it. Left in, this ships a
.d.tsthat fails on any downstream TS 5.9.x build, with no diagnostic on that build's own source.Fixed by hoisting to a named
MergedComputed<A, A2>. Naming it is only half: unexporting the alias was measured to reproduce the identicalTS2304, so it joinsBaseInstance/ConstructionKey/Sealed/EntityStatic/AbstractEntity/EntityUnionon the documented emit-nameability export list. Both halves are recorded as measurement inindex.tsandtypes.ts.examples/billing-domainemitted declarations and never type-checked them.TS4020is an emit-time diagnostic so that class was caught; a dangling type parameter in the output was not.typechecknow feeds the emitted files back through 5.9.3 — deliberately without--skipLibCheck, which was measured to make the step a no-op. That assumption is what let this through, and it is now written down inemit-guards.tsandCLAUDE.md.BillingDocumentBasealso gains a realcomputedfield (period, the accounting period a document falls in), so theTS7056margin is measured on the branch that spends most — the root's whole computed map serialises insideOmit<…>and again in& A2. Both compilers clean.Breaking, in two ways
Relaxing is no longer expressible.
immutable: []in a variant does not widenupdateInput. It breaks loudly —updateInputshrinks, so the patch call stops typechecking rather than changing behaviour silently. The migration is in the changeset: a field only some variants need locked comes off the root's list and goes on each variant that wants it locked.Entity.StaticandEntity.BaseInstancetake unions where they took tuples, so the empty case isneverrather than[]. Forced by theTS2344above and not fixable asymmetrically. The builders still constrain real call sites, so hand-written entity declarations are unaffected.minor, per 0.x.What got smaller
examples/billing-domain/src/index.tsis shorter:issuedAt/issuedTogone from both variants' lists, and the three-line comment teaching the old rule deleted rather than rewritten. That deletion is the whole point.Test plan
immutable: []no longer widensupdateInput, and a variant declaringcomputedkeeps the root'sgeneratedaccumulation on a root that generates; a variant redefining one computed key overrides that entry alonebase.test-d.tspins the type side — a variant cannot shed the root's immutable keys, and a redefined computed key reads as the variant's brand onEntity.Output, whichA & A2would failBehaviourOfis unmapped andTS2425blocks subtracting from it. Pre-existing, documented in the reference, asserted so it becomes a signal the day it changesexamples/billing-domain/src/index.spec.tspins the inheritance behaviourally: patchingissuedAtis refused though neither variant mentions itformat --check·lint·typecheck·test·knip·build— green in CI order, uncached, including the now-four-step consumer pass on 7.0.2 and 5.9.3One thing left alone
The merged fields are typed
S & S2while the runtime is child-wins — the same class of type lieMergedComputedfixes for the computed map, sitting two lines from the comment describing it. It is pre-existing, andOmit<S, keyof S2> & S2would spendTS7056budget on every entity rather than only those with computed fields. Recorded intypes.tsas known and why, rather than fixed here.🤖 Generated with Claude Code